Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

Issues: #23851

LldpNeighbors is covered, HostConverter will follow

NB: I tried to address all of the above, just the question related to brackets still stands

just the question related to brackets still stands

We don't have a policy for that, but I'd prefer brackets everywhere except when the method is called without any arguments.

Also extended host_discovery_test:

HostDiscoveredTest#test_0033_provision_interface isn't touched with no LLDP facts = 0.87 s = .
HostDiscoveredTest#test_0034_provision_interface isn't touched with no peer on the same VLAN = 0.77 s = .
HostDiscoveredTest#test_0036_provision_interface is not switched to bond0 if disabled = 0.77 s = .
HostDiscoveredTest#test_0035_provision_interface is switched to bond0 with more than one interface on the same VLAN = 1.00 s = .

There is one more:

HostDiscoveredTest#test_0037_former provision_interface is not managed after switching to bond0 = 0.95 s = .

@timogoebel: tests look good to me, please let me know in case you need more

ok to test

Don't blame ReferenceError: Can't find variable: jQuery (Capybara::Poltergeist::JavascriptError) on me ;-)

I checked this out. Making the changes in memory is actually pretty easy.

My naive approach looks like this:

 diff --git a/app/services/foreman_discovery/host_converter.rb b/app/services/foreman_discovery/host_converter.rb
index bcb1c05..25fa91e 100644
--- a/app/services/foreman_discovery/host_converter.rb
+++ b/app/services/foreman_discovery/host_converter.rb
@@ -33,26 +33,19 @@ class ForemanDiscovery::HostConverter

     ip = primary.ip
     name = primary.name
-    primary.update(
-       :primary   => false,
-       :provision => false,
-       :managed   => false,
-       :name      => nil,
-       :ip        => nil
-    )
+    primary.name = nil
+    primary.ip = nil
+    primary.provision = false
+    primary.primary = false

-    bond = Nic::Bond.create(
-        :identifier       => "bond0",
-        :attached_devices => neighbors,
-        :primary          => true,
-        :provision        => true,
-        :name             => name,
-        :ip               => ip,
-        :host             => host
+    bond = host.interfaces.build(
+      type: 'Nic::Bond',
+      identifier: 'bond0',
+      primary: true,
+      provision: true,
+      name: name,
+      ip: ip
     )
-
-    bond.save!
-    host.interfaces.push(bond)
   end

   def self.set_build_clean_facts(host)

This issue now is, that the primary interface cannot be found as rails thinks the relation is nil, see https://github.com/theforeman/foreman/blob/fcc2b55d07cbb3708f2fab212bf57a3366561d06/app/models/host/base.rb#L22
I have no idea yet, how we can workaround this.

@Thomas-Gelf: I like adding the idea of adding this to the fact parser so this code runs when the host is saved to the database. But I don't think it makes a lot of sense in core as the fact parser in core should just provide the data from a system so it can be displayed in Foreman's UI. In discovery we need this before provisioning happens. The fact parser prepares the host for proper provisioning.
We already have our own fact parser where we should be able to override the interfaces method to get this working.

@lzap, what do you think?

Overriding our parser is the best idea IMHO so far. :)

Update: I gave the fact_parser a try, shifted the whole logic - but failed. Have been unable to enforce a Nic::Bond and to pass attached_devices. Eventually I did it wrong, but anyways: I moved the call to eventually_make_bond() to Host::Discovered.populate_discovery_fields_from_facts. This way the transformation takes place immediately once the interfaces are being stored for the first time.

Hint: my attempt with the fact_parser was solely in foreman_discover, I didn't tweak the base class in foreman. So, by enhancing the possibilities there this should also be possible. However, the new approach looks good to me. @lzap: thoughts?

Rebased and fixed mac address setting

@lzap: I moved the code as requested and adjusted tests accordingly. Regarding the "post processing framework": could you please point me to an example Hook in the Foreman code, just to get an idea of how that should look like? In case it is not tricky I could give it a try.

I'd suggest to implement the "post processing framework" in a separate PR. It's not related to this issue. I agree it's a nice refactoring, but we don't need it now. @lzap, agreed?

Ok, although I think better direction would be creating post-processing framework.

I agree, luckily we can still do that later.

Please document the changes

Here you go: theforeman/theforeman.org#1129

Sure, I'd rather go that direction sooner because discovery plugin is basically a collection of hacks :-)

Thanks, @Thomas-Gelf.